Skip to content

feat(anc): direct HTTP download for hotfix with SHA-256 verification - #9233

Open
Abigail Liang (abigailliang-aks-sig-node) wants to merge 5 commits into
mainfrom
abigailliang/anc-hotfix-http-download
Open

feat(anc): direct HTTP download for hotfix with SHA-256 verification#9233
Abigail Liang (abigailliang-aks-sig-node) wants to merge 5 commits into
mainfrom
abigailliang/anc-hotfix-http-download

Conversation

@abigailliang-aks-sig-node

Copy link
Copy Markdown
Contributor

Summary

  • Add artifacts field to hotfix config JSON for direct HTTP download URLs + SHA-256 checksums
  • ~25x faster than apt-get/dnf path (0.87s vs 10-22s)
  • URL restricted to HTTPS + PMC allowlist, cross-domain redirects rejected
  • Backward compatible: falls back to apt/dnf when no artifacts field or no OS/arch match

Fallback strategy

Condition Behavior
artifacts missing or no OS/arch match Fallback to apt/dnf
HTTP network error Fallback to apt/dnf
SHA-256 mismatch Hard fail, keep VHD-baked ANC
Invalid URL (non-HTTPS, non-PMC) Hard fail, keep VHD-baked ANC

Example hotfix config with artifacts

{
  "hotfixes": {
    "202607.02": "202607.02.2"
  },
  "artifacts": {
    "202607.02.2": {
      "ubuntu-22.04-amd64": {
        "url": "https://packages.microsoft.com/...deb",
        "sha256": "c5c29cd3..."
      }
    }
  }
}

Future work (separate PRs)

  • Descriptor signing (schemaVersion, expiresAt, keyId)
  • LPS/ABSvc/CRP changes to preserve artifacts field during serialization
  • Package content validation (version/arch/OS inside package)

Test plan

  • All existing hotfix tests pass
  • New tests: artifact HTTP success, SHA mismatch hard fail, HTTP error fallback, no artifacts fallback, invalid URL hard fail
  • buildArtifactKey tests for Ubuntu/AzureLinux
  • validateArtifactURL tests for HTTPS/host allowlist
  • E2E with real PMC artifact

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Windows Unit Test Results

  3 files   12 suites   49s ⏱️
389 tests 389 ✅ 0 💤 0 ❌
392 runs  392 ✅ 0 💤 0 ❌

Results for commit a06a78b.

♻️ This comment has been updated with latest results.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a faster hotfix delivery path for aks-node-controller by allowing direct HTTPS artifact downloads (with SHA-256 verification) when the hotfix config includes an artifacts descriptor, while retaining the existing apt/dnf fallback behavior.

Changes:

  • Extend hotfix config parsing to support an artifacts map keyed by hotfix version and ID-VERSION_ID-GOARCH.
  • Implement direct HTTP download with URL allowlisting, redirect restrictions, and SHA-256 verification (with hard-fail on integrity violations).
  • Add unit tests covering artifact parsing, URL validation, key building, and download/fallback behaviors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
aks-node-controller/hotfix.go Adds artifact resolution + direct HTTP download/verify path and associated config structs/helpers.
aks-node-controller/hotfix_test.go Adds tests for artifacts parsing, artifact key derivation, URL validation, and download behavior.
aks-node-controller/checkhotfix.go Propagates/stages artifacts through check-hotfix (LPS + cold-start) into the shared pointer file.
aks-node-controller/app.go Adds an injectable httpDownload hook to allow unit tests to bypass real networking.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread aks-node-controller/hotfix.go Outdated
Comment thread aks-node-controller/hotfix.go Outdated
Comment thread aks-node-controller/hotfix.go
Comment thread aks-node-controller/hotfix_test.go Outdated
Comment thread aks-node-controller/hotfix_test.go Outdated
Comment thread aks-node-controller/checkhotfix.go

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

…fication

Replace the slow apt-get/dnf package manager path (~10-22s) with direct
HTTP download (~0.87s) when an artifact descriptor is present in the
hotfix config. The new optional `artifacts` field in the hotfix JSON
maps version + OS/arch to a download URL and SHA-256 digest.

Fallback strategy:
- No artifacts or no OS/arch match: fallback to apt/dnf (backward compat)
- HTTP network error: fallback to apt/dnf
- SHA-256 mismatch or invalid URL: hard fail, keep VHD-baked ANC
Three places were discarding the new artifacts field:
1. LPS staging (line 212): only carried Hotfixes
2. Cold-start fallback (line 448): lenient struct lacked Artifacts
3. writeHotfixConfig serialization: anonymous struct omitted Artifacts

All three now pass Artifacts through so download-hotfix can use
the direct HTTP download path end-to-end.
Add downloadDir field to App so tests can override the temp file
directory (CI lacks /opt/azure/containers/). Update test assertion
to expect the copyBinaryAlongside error (vhdBinaryPath not present)
while verifying package manager was not invoked.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

aks-node-controller/hotfix.go:641

  • When client.Do(req) returns an error, it can still return a non-nil resp. Not closing resp.Body in that case can leak connections/file descriptors (and prevent transport reuse).
	resp, err := client.Do(req)
	if err != nil {
		cancel()
		return nil, err
	}

aks-node-controller/checkhotfix.go:493

  • This only preserves existing.Artifacts when cfg.Artifacts is nil. If the incoming config has an empty-but-non-nil map (e.g. an explicit "artifacts": {} from an upstream serializer), this will overwrite any cloud-init populated artifacts with an empty object.
	// Preserve existing artifacts when the incoming config has none (e.g. LPS response
	// doesn't include artifacts yet). This mirrors the Version/ScriptsVersion preservation
	// and avoids erasing artifacts that cloud-init originally wrote.
	if out.Artifacts == nil {
		out.Artifacts = existing.Artifacts
	}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants